-
Notifications
You must be signed in to change notification settings - Fork 43
feat: streaming support #248
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@unleash = Unleash::Client.new( | ||
url: 'https://unleash.herokuapp.com/api', | ||
custom_http_headers: { 'Authorization': '943ca9171e2c884c545c5d82417a655fb77cec970cc3b78a8ff87f4406b495d0' }, | ||
url: 'https://app.unleash-hosted.com/demo/api', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-using sdk-examples setup https://github.com/Unleash/unleash-sdk-examples/blob/main/Ruby/.env.example as heroku is deprecated
368230d
to
7fb2b86
Compare
Unleash.engine.register_custom_strategies(Unleash.configuration.strategies.custom_strategies) | ||
|
||
Unleash.toggle_fetcher = Unleash::ToggleFetcher.new Unleash.engine | ||
Unleash.toggle_fetcher = Unleash::ToggleFetcher.new Unleash.engine unless Unleash.configuration.streaming_mode? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fetcher makes initial HTTP call on instance creation. We don't want that for how with streaming to avoid cross locks between streaming client and polling client
:bootstrap_config, | ||
:strategies, | ||
:use_delta_api | ||
:use_delta_api, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should deprecate this property as we have a standardized experimental mode in Node SDK that is either {type: 'streaming'} or {type: 'polling', format: 'delta'} or {type: 'polling', format: 'full'}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think having a standard with interface between language and SDK is too important here, most of the SDKs provide interfaces that are comfortable in that language.
I do agree that we should deprecate this and use your pattern though
lib/unleash/streaming_client.rb
Outdated
self.toggle_engine.take_state(event.data) | ||
end | ||
|
||
# TODO: update backup file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
state engine needs to expose current state so we can save it . Both streaming and polling with delta will need it
puts ">> START streaming.rb" | ||
|
||
@unleash = Unleash::Client.new( | ||
url: 'https://app.unleash-hosted.com/demo/api', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
demo doesn't have streaming enabled so this needs to be changed to enterprise URL
spec.require_paths = ["lib"] | ||
spec.required_ruby_version = ">= 2.7" | ||
|
||
spec.add_dependency "ld-eventsource", "2.2.4" unless RUBY_ENGINE == 'jruby' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SSE client has http dependency that doesn't work with jruby
).to be false | ||
end | ||
|
||
unless RUBY_ENGINE == 'jruby' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
run those tests for non jruby platforms
return nil if RUBY_ENGINE == 'jruby' | ||
|
||
begin | ||
require 'ld-eventsource' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't let this dependency spread throughout the code
Pull Request Test Coverage Report for Build 16880273865Details
💛 - Coveralls |
lib/unleash/streaming_client.rb
Outdated
end | ||
|
||
def start | ||
self.mutex.synchronize do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similar synchronization pattern to toggle_fetcher so that we guard running property
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to guard running? That should only ever be a problem if two threads are calling start and stop on the same instance in quick succession, surely?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No mutex needed in the scheduler for SSEs - lifecycle methods called from single thread. But in the event processor event processing happens from EventSource background threads that could potentially be concurrent so keeping it there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool! Come grab me next week and we can beat Yggdrasil into shape so you can finish this off!
:bootstrap_config, | ||
:strategies, | ||
:use_delta_api | ||
:use_delta_api, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think having a standard with interface between language and SDK is too important here, most of the SDKs provide interfaces that are comfortable in that language.
I do agree that we should deprecate this and use your pattern though
lib/unleash/configuration.rb
Outdated
end | ||
|
||
def streaming_mode? | ||
return false if RUBY_ENGINE == 'jruby' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should consider raising an exception here or logging an error. I would be very annoyed if an SDK I used didn't do what I told it and quietly did something different
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair point
lib/unleash/streaming_client.rb
Outdated
end | ||
|
||
def start | ||
self.mutex.synchronize do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to guard running? That should only ever be a problem if two threads are calling start and stop on the same instance in quick succession, surely?
@@ -1,15 +1,17 @@ | |||
require 'unleash/configuration' | |||
require 'unleash/toggle_fetcher' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like there's a missing somewhere here and I think that's related to the difference between the toggle fetcher internals and LD's event source lib. The toggle fetching code splits the logic for fetching toggles and the concurrency responsibility into two classes, whereas with the event source lib it's a single entity. This means we can't treat them like interchangable ducks and I really think we should be able to do that. It feels very wrong to have a :streaming_client
and :fetcher_scheduled_executor
exposed as accesors when we can only ever have one of these and they do the same job in different ways. It's leading to a bunch of places in this PR where we're checking which we and making local decisions. If we created something like a fetch_client
that composed of both the toggle_fetcher
and scheduled_executor
which exposes start and stop methods we could make a lot of that go away
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the root cause is who's driving the new updates: scheduler or streaming client handler. I will play around with a different split of responsibilities (as I did in the Java SDK) on Monday but I'm not sure we can have a drop-in replacement here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You comment was spot on. Thank you for that. I managed to create streaming_client_executor and streaming_event_processor. streaming_client_executor can play a role of a fetcher_scheduled_executor (kept the same field name in attr jus tin case someone depends on it). Please let me know if you like it more now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it!
module Unleash | ||
module Util | ||
module EventSourceWrapper | ||
def self.client |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'm a bit tempted to fail hard here rather than fall back to polling. I'm kinda okay with errors that happen on startup and I think it's better to force folks to set their stuff up correctly. We can always relax it later but if we do this now, I doubt we can enforce it later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking awesome. Gonna be great to get this one done!
About the changes
Adding SSE streaming client support in this SDK similar to Node SDK.
Important files
Discussion points